coordinate: address CodeRabbit feedback from #808#809
Conversation
1. server.go: parse cfg.TLS.AdditionalIPs into the IP set BEFORE
coordinate.SetupEtcdTLS() so the etcd server cert SANs include
user-configured addresses. Without this, distributed runners
reaching the cluster over an explicit AdditionalIP would fail
TLS verification against etcd.
2. coordinate.PublicIPs(): route through ComputeAdvertise so the
AutocertController DNS sanity check honors per-family netcheck
state and the CGNAT filter. Previously this path could leak the
netcheck source IP even when its family had zero reachable ports,
and the fallback admitted 100.64.0.0/10 addresses through the
bare IsGlobalUnicast/IsPrivate check.
3. debug_advertise: warn-and-continue on ipdiscovery failure
instead of aborting, mirroring server.go. The whole point of
the command is to diagnose discovery failure modes, so it
should still exercise the explicit-IP and netcheck paths
when interface discovery itself misbehaves.
4. debug_advertise: add --format json via the FormatOptions
pattern so the command produces machine-parseable output for
tooling/automation. Includes regenerated docs for the new flag.
Includes regenerated command docs for debug advertise.
|
No actionable comments were generated in the recent review. 🎉 ℹ️ Recent review info⚙️ Run configurationConfiguration used: Organization UI Review profile: CHILL Plan: Pro Run ID: 📒 Files selected for processing (4)
📝 WalkthroughWalkthroughThis PR refactors IP advertisement and discovery across three functional areas. The server startup moves explicit TLS IP parsing earlier to make those addresses available for certificate SAN configuration. The Coordinator's Comment |
|
lol I raced a session fixing coderabbit things w/ my own twitchy merge button pushes, and this is what i get |
2 participants
Follow-up to #808 (MIR-1018), which got merged before CodeRabbit's review was addressed. Four findings from CR, all valid; this PR works through them.
1.
cli/commands/server.go: parseAdditionalIPsbeforeSetupEtcdTLS. The IP set was being handed tocoordinate.SetupEtcdTLS()at line 299 with only the discovered IPs in it; explicitcfg.TLS.AdditionalIPsweren't parsed in until ~220 lines later. The etcd server cert SANs would silently omit user-configured addresses, so a distributed runner reaching the cluster over an explicitAdditionalIPwould fail TLS verification against etcd. Moved the parse up to right after IP discovery so the set is complete before any cert-issuing code reads it.2.
coordinate.PublicIPs(): route throughComputeAdvertise. This function feedsAutocertController.dnsPointsToUs, so getting it wrong means our ACME provisioning trusts addresses we shouldn't. The pre-refactor version had the same two leaksapiAddresses()had before MIR-1018: it returnedresp.SourceAddresseven when that family had zero reachable ports, and its fallback admitted100.64.0.0/10(CGNAT / Tailscale) through the bareIsGlobalUnicast/IsPrivatecheck. Routed it throughComputeAdvertiseand filtered to candidates classified asglobal-unicastso both gaps close in one move.3.
debug_advertise: warn-and-continue onipdiscoveryfailure. The command was aborting whereserver.gojust warns. That madedebug advertiseunable to reproduce the very failure mode it exists to diagnose. Mirroredserver.go's behavior so it falls through to the explicit-IP and netcheck paths with an empty discovery result.4.
debug_advertise: add--format json. StandardFormatOptionspattern for list-style commands. Useful for piping the candidate decisions into tooling. Suppresses the human progress logs in JSON mode so stdout is parseable; emits a{candidates: [...], advertised: [...]}document. Regenerated the command doc to pick up the new flag.Verified locally with
gotestsum ./components/coordinate(24 tests pass) andmake bin/miren && go generate ./...produces no diff after the commit.